Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate util/dev in favor of cargo alias #5109

Merged
merged 4 commits into from
Jan 31, 2020
Merged

Deprecate util/dev in favor of cargo alias #5109

merged 4 commits into from
Jan 31, 2020

Conversation

phansch
Copy link
Member

@phansch phansch commented Jan 30, 2020

This means one less shell script and a bit more cross-platform support
for contributors.

If you've been using ./util/dev before, this now becomes cargo dev.

The key part of this change is found in .cargo/config where an alias for calling the clippy_dev binary is defined.

changelog: none

@phansch
Copy link
Member Author

phansch commented Jan 30, 2020

Not sure if this will pass CI already and it will conflict with #5088 for now.

Also not sure about what this part in ./util/dev does, it may just be for CI?

CARGO_TARGET_DIR=$(pwd)/target/
export CARGO_TARGET_DIR

@flip1995
Copy link
Member

flip1995 commented Jan 30, 2020 •
edited
Loading

Don't worry about #5088, it's an easy change there

The env var makes sure, that clippy_dev gets build in the same target dir as clippy itself. This way libraries can be shared between clippy and clippy_dev. I think there is a cargo flag to also set this.

The env var also gets set by CI

tesuji
tesuji reviewed Jan 30, 2020
View reviewed changes
.cargo/config
@@ -1,5 +1,6 @@
[alias]
uitest = "test --test compile-test"
dev = "run --package clippy_dev --bin clippy_dev --manifest-path clippy_dev/Cargo.toml --"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use --target-dir flag I believe

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jan 30, 2020
@phansch
Copy link
Member Author

phansch commented Jan 30, 2020

Regarding the CARGO_TARGET_DIR value, I believe we don't need to set it anymore. What util/dev did, was setting it to be the same as the root project's target dir before cding into clippy_dev. When running cargo dev foo from the root project, it's already correct because we don't cd anymore.

@phansch
Deprecate util/dev in favor of cargo alias
4d1a11d 
If you've been using `./util/dev` before, this now becomes `cargo dev`.

The key part of this change is found in `.cargo/config`.

This means one less shell script and a bit more cross-platform support
for contributors.
@phansch
Move project_root function to clippy_dev/src/lib.rs
3036a2c 
This allows us to use the method in both `fmt.rs` and `lib.rs` in
multiple places. The downside is that we panic inside the method now,
instead of using the error handling in `fmt.rs`. We may want to
centralize the error handling for clippy_dev at some point, though.
@phansch phansch changed the title WIP: Deprecate util/dev in favor of cargo alias Deprecate util/dev in favor of cargo alias Jan 30, 2020
@phansch
Copy link
Member Author

phansch commented Jan 30, 2020

GitHub seems to show the wrong commit order: 3036a2c is the second commit and contains some more cleanup refactoring for finding the project root directory.

The PR should be ready for review now

@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 30, 2020
flip1995
flip1995 approved these changes Jan 30, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with last occurrence of copy of project_root() function removed.

clippy_dev/src/fmt.rs Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 30, 2020
@phansch
Copy link
Member Author

phansch commented Jan 31, 2020

@bors r=flip1995

@bors
Copy link
Contributor

bors commented Jan 31, 2020

📌 Commit 8f457fa has been approved by flip1995

bors added a commit that referenced this pull request Jan 31, 2020
@bors
Auto merge of #5109 - phansch:ciao_util_dev, r=flip1995
ca2abaf 
Deprecate util/dev in favor of cargo alias

This means one less shell script and a bit more cross-platform support
for contributors.

If you've been using `./util/dev` before, this now becomes `cargo dev`.

The key part of this change is found in `.cargo/config` where an alias for calling the `clippy_dev` binary is defined.

changelog: none
@bors
Copy link
Contributor

bors commented Jan 31, 2020

⌛ Testing commit 8f457fa with merge ca2abaf...

@bors
Copy link
Contributor

bors commented Jan 31, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing ca2abaf to master...

@bors bors merged commit 8f457fa into rust-lang:master Jan 31, 2020
@bors bors mentioned this pull request Jan 31, 2020
Merged
11 tasks
@phansch phansch deleted the ciao_util_dev branch January 31, 2020 08:43
@tesuji
Copy link
Contributor

tesuji commented Jan 31, 2020

When running cargo dev foo from the root project, it's already correct because we don't cd anymore

No, it is not. When unset CARGO_TARGET_DIR env, running cargo dev fmt will run clippy_dev/target/debug/clippy_dev, not target/debug/clippy_dev.

@tesuji tesuji mentioned this pull request Jan 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tesuji tesuji tesuji left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phansch @flip1995 @bors @tesuji